fix(schema-compiler): render MSSQL pushdown joins - #11778

• Gather context (PR diff, issue MSSQL SQL API pushdown omits joins from generated SELECT statements #11777, existing review threads)
• Read changed files in full (MssqlQuery.ts, mssql-query.test.ts)
• Analyze correctness of the pushdown-join fix
• Check test coverage & quality
• Verify build/tests locally — could not run, see Limitations
• Post review

The fix

MssqlQuery.ts:387 is correct, and I verified it end-to-end rather than by eye:

• It is a real omission, not an intentional divergence. The other two dialects that copy the whole statements.select template both render the loop: OracleQuery.ts:245 and PrestodbQuery.ts:204. MSSQL was the sole outlier. The line added is byte-identical to BaseQuery.js:4670.
• Clause position is right. It sits after the {% endif %} closing the from / from_prepared branch and before {% if filter %} — T-SQL requires joins after FROM and before WHERE. It also lands ahead of GROUP BY/HAVING/ORDER BY and the OFFSET/FETCH tail, none of which it perturbs.
• The rendered join is valid T-SQL. wrapper.rs:4507-4509 builds each join as query_aliased(join_sql, alias) → statements/join. query_aliased brackets the subquery in Rust (service.rs:1113-1114) before applying '{{ query }} AS {{ quoted_alias }}', and MSSQL overrides neither expressions.query_aliased nor join_types, so the output is LEFT JOIN (SELECT …) AS "a" ON …. LEFT/INNER/RIGHT/FULL are all valid T-SQL join type keywords.
• No regression risk on the Tesseract path. Tesseract's select() (rust/cube/cubesqlplanner/cubesqlplanner/src/planner/sql_templates/plan.rs:462-492) does not pass joins at all — it folds joins into from. With joins undefined, minijinja's default lenient undefined makes the loop render nothing, which BaseQuery's identical line already proves in production for every other dialect. So no existing MSSQL SQL snapshot changes.
• I diffed the whole MSSQL copy against base to check nothing else had drifted: ctes, distinct, select_concat, from/from_alias/from_prepared, filter, group_by, having, order_by are all present; the remaining differences (TOP placement, OFFSET/FETCH, OPTION (MAXRECURSION 0), no RECURSIVE keyword) are deliberate and documented. joins was the only gap.

Nothing to raise on security (no user input path — this is a static template string), performance, or docs (a dialect bug fix needs none; changelogs are Lerna-managed).

Findings

The two-line comment at mssql-query.test.ts:106-107 is fine — it is within budget and load-bearing (without it the assertion below reads as tautological and invites deletion).

Integration coverage

I checked whether an existing driver test could be unskipped as real proof. packages/cubejs-testing-drivers/src/tests/testQueries.ts:2914 (SQL API: SQL push down push to cube quoted alias) is the only suite test that pushes a LEFT JOIN down, but it is skipped for every dialect including Postgres (it needs IS NOT DISTINCT FROM), so it is not a candidate. No other skipped entry in fixtures/mssql.json produces a pushdown join. So the unit test really is the only available guard — which is exactly why finding 1 is worth acting on.

Limitations

I could not execute yarn test or yarn lint: node_modules is absent in this checkout and yarn install is outside my allowed tools. I verified the new assertions by hand against the template string instead — ') AS {{ from_alias }}' < 'FROM {{ from_prepared }}' < the joins loop < '{% if filter %}', so all four expectations hold and the test passes as written. Worth confirming CI is green before merge.

No prior review threads existed on this PR, so nothing to resolve or deduplicate against.

Verdict

The one-line change is right. MssqlQuery.ts:387 places {% for join in joins %}\n{{ join }}{% endfor %} after the {% endif %} that closes the {% if from %} / {% elif from_prepared %} pair and before {% if filter %}\nWHERE, byte-identical to BaseQuery.js:4670 and to the same line in OracleQuery.ts:245 and PrestodbQuery.ts:204. It resolves the symptom in #11777: without it the projection carries the join's alias while the JOIN that defines it is never emitted, and SQL Server answers The multi-part identifier "a.task_reference" could not be bound.

What I verified beyond the diff

• MSSQL was the only gap. Every statements.select definition in the repo — BaseQuery.js:4660, OracleQuery.ts:235, PrestodbQuery.ts:197, QuestQuery.ts:302, PinotQuery.ts:221 — already renders the loop. The other 13 dialects in src/adapter inherit BaseQuery's.
• joins is a SQL-API-pushdown-only variable, so the fix cannot regress Tesseract. SqlTemplates::select on the pushdown path passes it (rust/cubesql/cubesql/src/transport/service.rs:557), where each element is pre-rendered by wrapper.rs:4509 from the dialect's join + query_aliased templates. Tesseract's select (rust/cube/cubesqlplanner/cubesqlplanner/src/planner/sql_templates/plan.rs:462) never passes joins — it folds joins into from_prepared — so the new loop iterates an undefined value there, exactly as it already does for the four dialects that have carried this line all along. The test's header comment states this correctly.
• Placement is valid T-SQL. SELECT TOP n … FROM (…) AS x LEFT JOIN (…) AS y ON … is fine; MSSQL inherits query_aliased ({{ query }} AS {{ quoted_alias }}), which is the right derived-table spelling for T-SQL.
• The test's bare-prototype approach is sound. BaseQuery.sqlTemplates() (4559–4842) touches no this, and no dialect override does either, so sqlTemplates.call(Object.create(Cls.prototype)) is safe for all 17. This mirrors the existing union-template.test.ts and reuses its allDialects() helper.
• Tests and lint: green in CI, not run locally. This checkout has no node_modules and jest here runs against dist/, so I read CI on 7a78ac1 instead of installing: unit-core ✅ and lint ✅ (run 34984280364). That is the authoritative signal that the new test passes for all 17 dialects. I did not independently reproduce the failure against SQL Server — no MSSQL instance available here.
• Comment density in the new file is within the repo rule — two load-bearing comments, two lines and one line.

Findings

Both are posted inline with suggested directions. Neither affects the shipped fix.

Review threads

Nothing to reconcile: this PR has exactly two review threads, both on the now-deleted mssql-query.test.ts test, both already resolved after @davidda's replies confirming the move to select-template.test.ts. No stale threads of mine were left open, and neither new finding duplicates one.

Task list

• Read issue MSSQL SQL API pushdown omits joins from generated SELECT statements #11777 and the PR diff
• Read MssqlQuery.ts select template in full and compare with BaseQuery
• Check every other statements.select override in the repo, including driver packages
• Trace joins on both the pushdown and Tesseract paths in Rust
• Review the new test, its helper, and the bare-prototype assumption
• Confirm unit-core and lint green on the head commit
• Reconcile prior review threads
• Post findings

• Gather context (diff, issue MSSQL SQL API pushdown omits joins from generated SELECT statements #11777, changed files)
• Review MssqlQuery.ts template fix
• Review allDialects.ts + select-template.test.ts
• Verify the fix actually covers the reported symptom
• Reconcile prior review threads
• Post findings

The fix

packages/cubejs-schema-compiler/src/adapter/MssqlQuery.ts:387 is a byte-for-byte port of BaseQuery.js:4670, in the same position relative to the FROM branches and {% if filter %}. Traced both consumers to confirm the placement is the right one and that nothing else is missing:

statements/join is not overridden by MSSQL, and base's {{ join_type }} JOIN {{ source }} ON {{ condition }} plus expressions/query_aliased ({{ query }} AS {{ quoted_alias }}) are both valid T-SQL, so the loop is the only thing that was missing. MssqlQuery was also the last statements.select override in src/adapter without it — OracleQuery.ts:245 and PrestodbQuery.ts:204 already had it.

Findings

Low — test/unit/allDialects.ts:12-14 (thread): the reworded doc comment names two driver classes under "including", omitting the fact that makes those two the complete relevant set — they are the only driver packages that redefine statements.select. Seven other driver *Query subclasses inherit the base template and cannot escape the invariant. Suggested replacing the member list with the predicate.

Notes (not posted inline)

• test/unit/select-template.test.ts:21 — expect(select.search(fromPrepared)).toBeGreaterThan(select.search(fromAlias)) cannot fail once the two toMatch calls above it pass, since fromAlias's match ends on {% elif from_prepared %} and fromPrepared matches only that branch's body. Lines 19-23 also spell the same index two ways: select.search(joins) is joinsEnd.index. Cosmetic, and adjacent to a thread you have already settled — flagging, not asking.
• Residual risk, already accepted as out of scope on this thread: QuestQuery.ts:301 and PinotQuery.ts:220 are the only other copies of the base select template in the repo, i.e. exactly where "copy the base template, drop a clause" recurs, and they sit outside allDialects(). Both satisfy the new assertions today, so whenever a cross-package harness becomes cheap this is a free coverage win.

Verification caveat

Assertions above come from reading the templates and both renderers, not from a green run — yarn tsc, jest and oxlint were all denied in this environment, and packages/cubejs-schema-compiler/dist is absent, so yarn unit (which runs dist/test/unit) could not execute. The new test's regexes were traced by hand against all four distinct statements.select bodies (Base, MSSQL, Oracle, Presto) and match each, with the joins loop abutting the FROM {% endif %} and {% if filter %} so that both toBeGreaterThanOrEqual bounds hold at equality.

Review threads

The listing returned 4 threads opened by claude, all already resolved. Nothing to resolve this round; 1 new finding posted, 1 skipped as a duplicate of select-template.test.ts:6.

• Gather context (diff, issue MSSQL SQL API pushdown omits joins from generated SELECT statements #11777, changed files)
• Analyze the MssqlQuery template fix
• Analyze test changes (allDialects.ts, select-template.test.ts)
• Run relevant tests / lint — could not run, see below
• Review prior threads, resolve stale ones
• Post findings

The fix

packages/cubejs-schema-compiler/src/adapter/MssqlQuery.ts:387 is right, and it is the whole fix. statements.select is defined in exactly four places across the schema-compiler adapters, and I read all four:

MSSQL was the only dialect in the package missing it, so all 17 now satisfy the invariant. Placement matches the other three: after the {% endif %} closing both FROM branches, before {% if filter %}.

Two things I checked that could have made this the wrong fix:

No pushdown guard exists. contains_template is consulted for expressions/interval, expressions/cast, types/nullable and expressions/extract_epoch_diff — never for joins. rust/cubesql/cubesql/src/transport/service.rs:553 renders statements/select with joins => joins unconditionally, so a dialect that drops the loop emits SQL projecting an alias it never bound rather than declining the pushdown. That is exactly the The multi-part identifier "a.task_reference" could not be bound. in #11777, and adding the loop is the correct direction rather than gating the rewrite.

No Tesseract regression. rust/cube/cubesqlplanner/cubesqlplanner/src/planner/sql_templates/plan.rs:477 renders the same template with from_prepared and no joins binding at all. minijinja is left at its default UndefinedBehavior::Lenient (no set_undefined_behavior call anywhere in rust/), so the loop iterates empty. The four dialects above already prove this in production.

The TOP / OFFSET…FETCH and OPTION (MAXRECURSION 0) tails MSSQL adds around the base template don't interact with the inserted line.

Tests

select-template.test.ts is a reasonable guard for a template-only defect. It fails on MSSQL without the fix, and the regexes are formatting-tolerant enough to match all four definitions — including Oracle's ) {{ from_alias }} with no AS, and Presto's extra space before {% elif %}. Reading templates off Object.create(QueryClass.prototype) is the pattern union-template.test.ts already established, and it is safe here: the one dialect whose sqlTemplates() branches on instance state, packages/cubejs-schema-compiler/src/adapter/MysqlQuery.ts:267, reads this.useGeneratedTimeSeries as undefined and takes the falsy branch rather than throwing.

QuestQuery and PinotQuery also override statements.select and both already carry the loop, so nothing is broken outside the scan — but nothing is guarding them either. The doc comment acknowledges this and expanding discovery across packages is fairly scoped out of an MSSQL fix.

Finding

One low-severity issue posted inline on packages/cubejs-schema-compiler/test/unit/allDialects.ts:13-14: the reworded JSDoc's enumeration of out-of-scan driver classes holds for statements.select but not for statements.union, where DruidQuery and KsqlQuery also escape. Comment accuracy only — no behavioural impact.

Not verified

yarn install is not permitted in this sandbox and the checkout has no node_modules, so I did not run select-template.test.ts, yarn tsc, or yarn lint. Everything above is static reading of the four template definitions and the two Rust render sites. CI results should be treated as the authority on whether the new test passes for all 17 dialects.

Review threads

The listing returned 0 unresolved threads (0 of them mine) across both pages — the earlier allDialects documentation thread and the select-template.test.ts:11 anchors thread davidda pushed back on are both already resolved. Nothing to resolve, nothing re-affirmed; the finding above posts as a fresh thread.

• Read PR diff and referenced issue MSSQL SQL API pushdown omits joins from generated SELECT statements #11777
• Read changed files in full context
• Verify the fix covers the reported symptom
• Assess test quality / coverage
• Reconcile prior review threads
• Post findings

Verdict

packages/cubejs-schema-compiler/src/adapter/MssqlQuery.ts:387 adds exactly the loop BaseQuery.js:4670 has, in the same slot — after the {% endif %} that closes both FROM branches, before {% if filter %}\nWHERE. That is the required T-SQL clause order, and it resolves the The multi-part identifier "a.task_reference" could not be bound. symptom in #11777.

Confirmed the two consumers behave as the test's header comment claims:

• rust/cubesql/cubesql/src/transport/service.rs:554 renders statements/select with joins => joins — this is the SQL API pushdown path that was broken.
• rust/cube/cubesqlplanner/.../sql_templates/plan.rs:477 passes from_prepared and no joins — Tesseract folds joins into FROM, which is why no snapshot covered this.

Checked the other dialects that override statements.select: OracleQuery.ts:245 and PrestodbQuery.ts:204 already render the loop, as do PinotQuery.ts:230 and QuestQuery.ts:311 in the driver packages. MSSQL was the only gap.

Test

select-template.test.ts follows the established union-template.test.ts pattern (templates read off a bare prototype via allDialects()), which is the right call here — a query-level snapshot would not have caught this, since Tesseract never supplies joins.

Traced the four regexes by hand against every schema-compiler statements.select (the three overrides plus BaseQuery's, inherited by the other 14):

Drop the one-line fix and only MssqlQuery fails, on expect(select).toMatch(joins). Discovery resolves to dist/src/adapter under yarn unit (jest --coverage dist/test/unit), where \w+Query\.(ts|js) matches the 17 compiled dialects and skips .d.ts, QueryBuilder, QueryCache, QueryFactory — comfortably over the MIN_DIALECTS = 10 floor.

Not executed: the package has no node_modules or dist in this checkout, so yarn unit and yarn lint were not run. The table above is static analysis of the template strings, not a test run. CI covers both.

Finding

1. low — select-template.test.ts:19-23: the ordering assertions recompute offsets already held in fromEnd.index / joinsEnd.index. Suggestion posted inline; no behaviour change.

Not raised inline

allDialects() still does not reach PinotQuery / QuestQuery, the two statements.select overrides living in driver packages — both carry the joins loop today, so nothing is broken, and you already scoped that out. Noting it only so the gap stays visible: those two sit in exactly the unguarded state that produced #11777.

I also withdrew a concern about the fromAlias / fromPrepared anchors pinning the exact FROM-branch shape. Your argument holds — anchoring on from_prepared alone would pass with the joins loop nested inside the FROM conditional, which is a real way to reintroduce this bug.

Review threads

Listing returned 1 thread of mine (single page). Resolved 1 (PRRT_kwDOCOH19M6jSKzd, allDialects.ts — the suggested wording is in the file verbatim at lines 12-14). 0 re-affirmed.

• Gather context (PR diff, issue MSSQL SQL API pushdown omits joins from generated SELECT statements #11777)
• Read changed files in full
• Verify the fix against the reported symptom
• Audit every other statements.select override
• Review test coverage & quality
• Handle prior review threads
• Post findings

Fix

packages/cubejs-schema-compiler/src/adapter/MssqlQuery.ts:387 adds the join loop byte-for-byte identical to BaseQuery.js:4670, in the same slot — after the {% if from %}/{% elif from_prepared %}/{% endif %} block, before {% if filter %}\nWHERE. That is the only legal slot in T-SQL, and the one TOP / OFFSET…FETCH / OPTION (MAXRECURSION 0) don't interact with.

Traced both consumers of the template to confirm the diagnosis in the issue and the claim in the test's header comment:

• rust/cubesql/cubesql/src/transport/service.rs:514-557 — the SQL API's select() takes joins: Vec<String> and passes joins => joins unconditionally. Nothing checks that the dialect's template reads it, which is why the omission surfaced as The multi-part identifier "a.task_reference" could not be bound. rather than a planner error.
• rust/cube/cubesqlplanner/cubesqlplanner/src/planner/sql_templates/plan.rs:462-493 — Tesseract's select() has no joins parameter at all; it renders each join through statements/join (plan.rs:495) and folds the result into from_prepared. So the loop is inert under Tesseract, and ctes/joins are mutually exclusive across the two paths — the new loop cannot interact with the OPTION (MAXRECURSION 0) tail.

Completeness against the symptom: MSSQL's template now consumes every input the SQL API passes. The one base input it still omits is recursive, which is correct — T-SQL has no WITH RECURSIVE.

Other dialects overriding statements.select

MSSQL was the only gap. Checked all four other overrides; each already has the loop in the same position:

The last two sit outside allDialects()' scan, which is exactly what the reworded doc at packages/cubejs-schema-compiler/test/unit/allDialects.ts:11-17 now says. Since both render the loop correctly today, the uncovered set holds no live defect, and extending discovery across packages stays out of scope as agreed.

Test

packages/cubejs-schema-compiler/test/unit/select-template.test.ts follows the established union-template.test.ts pattern — templates read off a bare prototype, asserted over allDialects(). Hand-checked the four regexes against each template that exists in the scanned directory (base, Oracle, Prestodb, MSSQL): all four match, and both ordering assertions hold, including Oracle's ) {{ from_alias }} with no AS and Prestodb's extra space before {% elif %}. Dropping the one-line fix makes expect(select).toMatch(joins) fail for MssqlQuery and nothing else, so the test does gate the regression it was written for. No dialect deletes statements.select, so the unconditional assertion (unlike the union test's opt-out branch) is correct.

The comments in the new file are 2 lines and 1 line — within budget, and both load-bearing: the first records which planner reads the loop, which is the fact that makes the test necessary at all.

Not verified by execution: yarn install has not run in this checkout (no node_modules/.bin/jest, no packages/cubejs-schema-compiler/dist) and the sandbox blocks node, so I could not run the suite. The regex verification above is by hand against the template strings, not a test run. CI is the authority on the run itself.

Review threads

The thread listing returned 0 unresolved threads (0 of them mine) — every thread from earlier rounds is already resolved, so nothing to resolve and nothing to re-affirm. Two positions from earlier rounds are recorded here rather than on their (already-resolved) threads: the FROM-branch anchors at select-template.test.ts:10-11 remain a coupling I'd have written differently, but the author's rationale holds and it is not worth another round; the cosmetic select.search(...) re-scan at select-template.test.ts:19-23 I withdraw outright — the author is right that the forms are equivalent.

Nit, non-blocking

Commit 5c6a6f0 is titled Update packages/cubejs-schema-compiler/test/unit/allDialects.ts, which isn't a conventional-commit subject per the repo's CLAUDE.md. Harmless if this lands as a squash under the PR title.